Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert EuiIcon to TypeScript #1355

Merged
merged 7 commits into from
Dec 9, 2018

Conversation

pugnascotia
Copy link
Contributor

Summary

Convert the EuiIcon component to TypeScript. Notes:

  • I switched the invocation of tsc and tslint, as TSLint recommends that compilation run cleanly before linting.
  • I added a directory for custom library definitions, so that TS wouldn't complain about importing SVGs. These imports are still compiled into inline React components with Babel.
  • I removed the icon title tests, since they have been deprecated and wrong for a long time.

Something else to note is that I'm seeing the following warning when building EUI now:

[BABEL] Note: The code generator has deoptimised the styling of /Users/rory/src/elastic/eui/src/components/icon/icon.tsx as it exceeds the max of 500KB.

True enough, if you look at the output in lib/, it's...ugly. I ran it through Prettier, and did the same for master, and the output varies in the expected ways but not by that much. Perhaps I just inched over the threshold? I did wonder if we should stop using the plugin and write a script to call svgo and generate actual component files instead? In any case, I started the docs site locally and the icons looked fine.

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

package.json Show resolved Hide resolved
src/components/icon/icon.tsx Outdated Show resolved Hide resolved
src/components/icon/icon.tsx Outdated Show resolved Hide resolved
if (COLORS.indexOf(color) > -1) {
optionalColorClass = colorToClassMap[color];
if (color) {
checkValidColor(color, type || 'empty');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting way to re-implement this prop type, I like it!

src/components/icon/icon.tsx Show resolved Hide resolved
src/components/icon/icon.test.tsx Outdated Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

That babel warning is fine. It's on our roadmap to begin addressing EuiIcon's overall size.

@pugnascotia
Copy link
Contributor Author

@chandlerprall I've pushed some review feedback, but the prop types generator fails on these again. Might be the keysOf change - would you mind taking a look?

@pugnascotia
Copy link
Contributor Author

Cancel that - everything's good after rebasing, so I've also inlined that interface that I mentioned above. 🙌

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

@pugnascotia pugnascotia merged commit 244a779 into elastic:master Dec 9, 2018
@pugnascotia pugnascotia deleted the convert-icon-to-ts branch December 9, 2018 12:29
@snide snide mentioned this pull request Dec 11, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants